Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix svg dependencies not being found when using minification #1022

Merged
merged 5 commits into from
Mar 20, 2018

Conversation

DeMoorJasper
Copy link
Member

Adds a walker as a plugin that collects svg dependencies before htmlnano is being ran, this enables svg dependency collecting as svgo stringifies the entire svg ast tree removing our ability to walk through it.

Closes #1019

srcset: ['img', 'source'],
poster: ['video'],
'xlink:href': ['use'], // Deprecated since SVG 2, throws error in svgo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI Chrome doesn’t support the plain href so it can be “deprecated” but support is 0 as far as I’m concerned. Does this change still work?

Copy link
Member Author

@DeMoorJasper DeMoorJasper Mar 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still supported in this PR, don't wanna break cross-browser support.
Dependency collecting (for svgs) is just moved from collectDependencies into svgDependencyWalker in this pr (as htmlnano stringifies svgs...)
Therefore it shouldn't be in the Object as walking over a dependency twice would cause parcel to error as the bundles don't exist yet when this is running.

@devongovett
Copy link
Member

Instead of introducing a separate dependency walker just for SVG, we could do this the same way JSAsset does it. There are both pretransform and transform hooks: pretransform happens before collecting dependencies and should apply posthtml, and transform happens after collecting deps and should apply htmlnano.

@codecov-io
Copy link

Codecov Report

Merging #1022 into master will increase coverage by 3.43%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1022      +/-   ##
==========================================
+ Coverage   86.35%   89.79%   +3.43%     
==========================================
  Files          68       70       +2     
  Lines        3541     3617      +76     
==========================================
+ Hits         3058     3248     +190     
+ Misses        483      369     -114
Impacted Files Coverage Δ
src/transforms/posthtml.js 100% <ø> (+19.51%) ⬆️
src/assets/HTMLAsset.js 97.95% <100%> (+10.72%) ⬆️
src/transforms/htmlnano.js 100% <100%> (ø)
src/assets/GraphqlAsset.js 100% <0%> (ø) ⬆️
src/utils/getCertificate.js 85.71% <0%> (ø)
src/visitors/dependencies.js 87.01% <0%> (+1.29%) ⬆️
src/utils/config.js 88.4% <0%> (+1.44%) ⬆️
src/Server.js 93.15% <0%> (+1.48%) ⬆️
src/HMRServer.js 92.3% <0%> (+1.83%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc654d7...4759158. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants